-
Notifications
You must be signed in to change notification settings - Fork 460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add missing packages to the list of clients #616
Conversation
) | ||
|
||
// API is the Stripe client. It contains all the different resources available. | ||
type API struct { | ||
// Account is the client used to invoke /accounts APIs. | ||
Account *account.Client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why it says Account
and not Accounts
? Should we fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'd guess because maybe the original package started out by just doing GET /account
, which was singular.
Yeah, I think we should fix it, but maybe not now since it's breaking — maybe add a TODO or something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I think we can punt on the TODO and I'll change it when we go ahead and remove the old 3DS endpoint
// Orders is the client used to invoke /orders APIs. | ||
Orders *order.Client | ||
// OrderReturns is the client used to invoke /order_returns APIs. | ||
OrderReturns *orderreturn.Client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was not sure if Orders
was before OrderReturns
(as in whether the case matters in alphabetical ordering). It seemed easier/cleaner this way though. (true for other packages like sub below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically I think OrderReturns
would come first. I don't think it matters that much though — like you said, this will be more of a sane ordering for users reading through this list.
// SubscriptionItems is the client used to invoke subscription's items related APIs. | ||
SubscriptionItems *subitem.Client | ||
// Tokens is the client used to invoke /tokens APIs. | ||
Tokens *token.Client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not add threedsecure
since it's been deprecated for well over a year. Is it the right call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if no one's asked for it by now, let's just leave it out.
Minor comment about possibly adding a TODO comment or something above, but otherwise LGTM. Thanks for the quick fix. ptal @remi-stripe |
I think we should punt on the TODO and fix later, I'll take note of it so that we don't forget it. |
Works for me. |
Bumps [sorbet](https://github.com/sorbet/sorbet) from 0.5.10206 to 0.5.10263. - [Release notes](https://github.com/sorbet/sorbet/releases) - [Commits](https://github.com/sorbet/sorbet/commits) --- updated-dependencies: - dependency-name: sorbet dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This PR starts by re-ordering all packages alphabetically to make it easier to maintain. It then adds all the packages that were missing from the list.
This fixes #614
r? @brandur-stripe
cc @stripe/api-libraries